Skip to content

Conversation

crazyair
Copy link
Contributor

@crazyair crazyair commented Sep 3, 2025

Summary by CodeRabbit

  • 新增文档与示例
    • 添加“列表字段卸载/隐藏”示例与说明,演示在动态列表中切换字段可见性并查看提交行为及当前表单值展示。
  • 修复
    • 改进动态列表场景下表单值提取与验证逻辑,避免已卸载字段被错误计入,提升提交结果准确性。
  • 测试
    • 新增针对列表卸载与列表级校验的单元测试,覆盖可见性切换与校验失败场景。

Copy link

coderabbitai bot commented Sep 3, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

新增文档与示例,演示在 Form.List 中按条件卸载字段;调整表单校验中对 list 项路径的最终取值逻辑(引入去重快照用于取值),并新增针对列表卸载与列表级规则的测试用例。公开接口未变更。

Changes

Cohort / File(s) Summary
Docs & Demo
docs/demo/list-unmount.md, docs/examples/list-unmount.tsx
新增文档条目与 React TypeScript 示例:展示在 Form.List 中基于 isShow 条件渲染/卸载年龄字段,包含切换按钮、表单展示与提交日志(导出默认组件 Demo)。
Core Form Logic
src/useForm.ts
调整 validateFields 流程:在遍历字段时提前计算 fieldNamePath,引入 finalValueNamePathList(快照)用于最终取值,遍历时对 list 项路径做包含匹配去重以避免重复加入同一 list-item 路径;其余验证流程与返回逻辑保持一致。
Tests
tests/list.test.tsx
在测试中添加 useState 导入,新增两项测试:list unmount(验证动态卸载字段后提交值形状)与 list rules(验证 Form.List 级别校验触发 onFinishFailed)。

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User as 用户
  participant Demo as Demo 组件
  participant Form as Form 层
  participant Store as FormStore

  User->>Demo: 切换 隐藏/显示 age 字段
  Demo->>Form: 更新 state / 重新渲染
  Form-->>Demo: 挂载或卸载 age 字段

  User->>Demo: 点击 提交
  Demo->>Form: 调用 validateFields()
  Form->>Store: 转发校验请求

  rect rgba(220,245,220,0.18)
    note right of Store: 校验与取值关键变更
    Store->>Store: 遍历字段,计算 fieldNamePath
    Store->>Store: 构建 finalValueNamePathList(快照)
    Store->>Store: 对 list-item 路径做包含匹配去重,避免重复加入
    Store->>Store: 基于快照获取最终字段值并执行校验
  end

  Store-->>Form: 返回校验结果(成功/失败)
  Form-->>Demo: 返回 Promise,触发 onFinish 或 onFinishFailed
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • zombieJ

Poem

嗅嗅草间小径,🐇
列表影里忽隐明。
显与隐,一键分清,
校验轻扫不重名。
我在萝卜旁悄悄颂。

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed 该标题简洁明确地指出此次 PR 核心变更——修复 Form.List 在 onFinish 回调中未正确返回值的问题,与实际提交的 src/useForm.ts 中对 validateFields 和 getFieldsValue 的修改高度契合,同时未添加无关信息。
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4e69d82 and 8c884da.

📒 Files selected for processing (1)
  • src/useForm.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/useForm.ts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

vercel bot commented Sep 3, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
field-form Ready Ready Preview Comment Sep 29, 2025 0:38am

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @crazyair, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

此拉取请求旨在解决 rc-field-form 中一个关键的表单值收集问题,即 Form.List 的名称在 onFinish 回调中未能正确获取。通过修改核心表单逻辑,确保即使列表中的字段被动态卸载,表单也能准确地捕获所有相关数据。此外,还引入了一个新的示例来验证并展示此修复。

Highlights

  • 修复 Form.List 名称在 onFinish 中取值不正确的问题: 解决了当 Form.List 中的字段被条件渲染或卸载时,其名称无法正确包含在 onFinish 回调返回的值中的问题。
  • 新增 Form.List 卸载示例: 添加了一个新的文档和示例,用于演示和测试 Form.List 在其内部字段被卸载时的行为,确保数据收集的正确性。
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

此拉取请求旨在修复当 Form.List 中的字段被卸载后,其旧值仍会包含在 onFinish 回调的表单值中的问题。核心改动是在 validateFields 方法中,当收集所有字段路径用于最终取值时,跳过 Form.List 自身的实体,仅依赖其已挂载的子字段实体来构建值。这能正确处理字段卸载的场景。然而,这个修复引入了一个严重的问题:当 Form.List 为空数组时,它在 onFinish 的结果中会丢失。我已经就此问题在 src/useForm.ts 中添加了具体的审查评论。

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (6)
src/useForm.ts (2)

1003-1011: 确保返回值与 “不包含 List 名称” 目标一致并覆盖测试

此处 returnPromise 最终通过 this.getFieldsValue(namePathList) 生成 values。由于上面已跳过 List 容器,namePathList 仅包含叶子字段,最后合并出的对象会包含 users 的值树,但不会把 List 自身当作字段实体返回,符合预期。建议新增一个用例覆盖:

  • 表单含 Form.List name="users",仅选中子字段时,onFinish(values) 中没有单独的 “List 容器字段” 项,但 values.users 的数组结构完整。

我可以基于现有测试框架起草一个最小复现用例。


309-351: getFieldsValue 的 List 过滤语义与 validateFields 对齐性

这里在 strict 与非 strict 下分别跳过 isListisListField。从行为上看与本次改动一致,但建议在源码注释中明确两者差异(List 容器 vs. List 子字段),避免后续维护者误用;同时补一条单测覆盖 getFieldsValue({ strict: true }) 与默认模式的差异,特别是在条件卸载子字段时的表现。

docs/demo/list-unmount.md (1)

1-3: 标题与其他示例保持命名风格一致

建议将小写的 ## list 调整为 ## List(或中文标题,如“列表:条件卸载字段”),与文档其它章节风格统一,便于检索。

docs/examples/list-unmount.tsx (3)

25-25: 演示区块可读性小优化

<Form.Field shouldUpdate> 直接渲染 JSON 字符串可读性一般,建议包裹在 <pre> 以保留缩进格式,或使用一个只读文本区域,便于对比 strict 与非 strict 的差异。

-        <Form.Field shouldUpdate>{() => JSON.stringify(form.getFieldsValue(), null, 2)}</Form.Field>
+        <Form.Field shouldUpdate>
+          {() => <pre>{JSON.stringify(form.getFieldsValue(), null, 2)}</pre>}
+        </Form.Field>

33-41: 利用 strict 对比“已注册字段 vs. 保留值”的效果非常好,建议在示例内补一行注释说明意图

当前示例通过条件卸载 age 字段并对比 getFieldsValue()getFieldsValue({ strict: true }),很好地演示了差异。为减少读者困惑,建议在此处添加一句注释说明:当 age 被卸载时,默认模式仍会返回保留的值,而 strict 只返回当前已挂载字段的值。


47-49: 按钮文案更明确一些

将“隐藏”调整为“隐藏/显示年龄”或增加 aria-pressed,对可读性与无障碍更友好。

-        <button type="button" onClick={() => setIsShow(c => !c)}>
-          隐藏
-        </button>
+        <button
+          type="button"
+          aria-pressed={!isShow}
+          onClick={() => setIsShow(c => !c)}
+        >
+          隐藏/显示年龄
+        </button>
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 2b0f66b and e37a002.

📒 Files selected for processing (3)
  • docs/demo/list-unmount.md (1 hunks)
  • docs/examples/list-unmount.tsx (1 hunks)
  • src/useForm.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
docs/examples/list-unmount.tsx (1)
tests/common/InfoField.tsx (1)
  • Input (9-11)
🔇 Additional comments (2)
src/useForm.ts (1)

924-926: 验证 Form.List 容器定向校验兼容性
本地运行完整测试并确认以下场景符合预期:

  • 调用 validateFields(['users']) 且 recursive: false 时是否跳过子项
  • recursive: true 或传入父/子路径时的校验行为
    在文档中明确说明不支持对 List 容器直接配置校验规则。
docs/examples/list-unmount.tsx (1)

1-5: 示例组件依赖已存在
已在 docs/examples/components 下找到 Input.tsxLabelField.tsx,文档站构建不会因缺失此处组件而失败。

Copy link

codecov bot commented Sep 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.53%. Comparing base (2b0f66b) to head (8c884da).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #761   +/-   ##
=======================================
  Coverage   99.53%   99.53%           
=======================================
  Files          19       19           
  Lines        1287     1292    +5     
  Branches      319      318    -1     
=======================================
+ Hits         1281     1286    +5     
  Misses          6        6           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zombieJ
Copy link
Member

zombieJ commented Sep 5, 2025

@gemini-code-assist Review it

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

这个 PR 修复了在 Form.List 中,已卸载的字段仍然会被包含在 onFinishvalues 中的问题。通过在 useForm.ts 中改用 getFieldsValue({ strict: true }),确保了在表单校验成功后,只收集已挂载字段的值,正确地解决了这个问题。新增的 list-unmount.tsx 示例和相关文档非常棒,清晰地展示了修复效果。

不过,这个修改也可能改变了 validateFields 的行为,它现在总是返回所有字段的值,而不是像以前一样只返回被校验字段的值。我在代码中留了一条评论,对此进行了更详细的说明,并指出了成功和失败路径下取值逻辑可能存在的不一致性。请查看。

@zombieJ
Copy link
Member

zombieJ commented Sep 17, 2025

@gemini-code-assist, pls help to CR again

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

本次 PR 修复了 Form.List 在动态表单场景下,因列表项卸载导致 onFinish 回调中表单值不正确的问题。同时添加了对应的文档和示例。

代码改动主要在 src/useForm.tsvalidateFields 方法中,通过避免重复添加列表字段路径来解决问题。

整体来看,PR 的目标明确,修复是有效的。我有一个关于路径匹配逻辑的建议,希望能让代码更健壮。

@crazyair
Copy link
Contributor Author

@gemini-code-assist Review it

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

本次 PR 修复了在 Form.List 中动态卸载字段时 onFinish 可能获取到不正确值的问题。核心改动在 useForm.ts 中,通过调整 validateFields 方法里的字段收集逻辑,避免了列表字段和其子字段路径被重复添加,从而保证了取值的正确性。同时,PR 也新增了对应的单元测试和文档示例,覆盖了修复的场景。整体来看,代码改动清晰,测试和文档补充也很到位。我在 useForm.ts 中留下了一条关于代码健壮性的建议,旨在提高代码的长期可维护性,请查阅。

@zombieJ
Copy link
Member

zombieJ commented Sep 24, 2025

@gemini-code-assist Review it

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new example demonstrating the handling of dynamic lists with conditional field visibility in the rc-field-form library. It also fixes a bug related to form validation and submission in dynamic list scenarios, ensuring more stable and accurate results. The changes include adding a new documentation file, a new example component, and modifications to the useForm.ts file to address the bug. Additionally, a new test case is added to verify the fix.

Comment on lines 925 to 933
if (!provideNameList) {
namePathList.push(field.getNamePath());
if (field.isList() && namePathList.some(name => matchNamePath(name, fieldNamePath, true))) {
removeListNameStrList.push(fieldNamePath.toString());
}
namePathList.push(fieldNamePath);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This logic seems a bit complex. It might be beneficial to add a comment explaining the purpose of this if condition and how it contributes to resolving the issue. Specifically, clarify why you're checking if field.isList() and if namePathList already contains a matching name path, and why this necessitates removing the list name.

src/useForm.ts Outdated
Comment on lines 1009 to 1011
const filterListNameList = namePathList.filter(
name => !removeListNameStrList.some(nameStr => nameStr === name.toString()),
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

It would be helpful to add a comment explaining why removeListNameStrList is being used to filter namePathList. What is the purpose of filtering these names, and what problem does it solve?

Comment on lines +941 to +1002
it('list unmount', async () => {
const valueRef = React.createRef();

const Demo = () => {
const [isShow, setIsShow] = useState(true);
return (
<Form
initialValues={{
users: [
{ name: 'a', age: '1' },
{ name: 'b', age: '2' },
],
}}
onFinish={values => {
valueRef.current = values;
}}
>
<Form.List name="users">
{fields => {
return fields.map(field => (
<div key={field.key} style={{ display: 'flex', gap: 10 }}>
<InfoField name={[field.name, 'name']}>
<Input />
</InfoField>
{isShow && (
<InfoField name={[field.name, 'age']}>
<Input />
</InfoField>
)}
</div>
));
}}
</Form.List>
<button data-testid="hide" type="button" onClick={() => setIsShow(c => !c)}>
隐藏
</button>
<button type="submit" data-testid="submit">
Submit
</button>
</Form>
);
};

const { queryByTestId } = render(<Demo />);
fireEvent.click(queryByTestId('submit'));
await act(async () => {
await timeout();
});
expect(valueRef.current).toEqual({
users: [
{ name: 'a', age: '1' },
{ name: 'b', age: '2' },
],
});

fireEvent.click(queryByTestId('hide'));
fireEvent.click(queryByTestId('submit'));
await act(async () => {
await timeout();
});
expect(valueRef.current).toEqual({ users: [{ name: 'a' }, { name: 'b' }] });
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This is a good test case for the unmount scenario. Consider adding comments within the test to explain the different steps and assertions, making it easier to understand the test's purpose and logic.

src/useForm.ts Outdated
if (!provideNameList) {
namePathList.push(field.getNamePath());
if (field.isList() && namePathList.some(name => matchNamePath(name, fieldNamePath, true))) {
removeListNameStrList.push(fieldNamePath.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个逻辑很奇怪,这里 matchNamePath 一次后,下面又从列表里匹配一遍。
应该是提取一个 filledPathList 专门存需要留档的路径出来,避免多轮遍历。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

要用2个 list吗?也行吧,就是 push 的地方要写好几处地方,比较麻烦。

我现在实现的,就是单纯记录 List 这层的 name,然后在提交时候过滤。

@zombieJ
Copy link
Member

zombieJ commented Sep 28, 2025

@gemini-code-assist pls help to review again and provide more detail action.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

本次 PR 修复了 Form.List 中动态卸载的字段值仍会出现在 onFinish 回调中的问题。实现方式是通过在 validateFields 方法中区分“需要校验的字段列表”和“最终提交时需要取值的字段列表”,确保在提交时只包含当前已挂载的字段值。

改动逻辑清晰,并且通过新增的示例代码和单元测试充分覆盖了修复的场景,确保了代码质量。我对其中一处实现的复杂度和潜在性能问题提出了改进建议,但总体而言,这是一次高质量的提交。

Comment on lines 928 to 936
if (!provideNameList) {
namePathList.push(field.getNamePath());
if (
// When Form.List has a value, filter Form.List `name`
!(field.isList() && namePathList.some(name => matchNamePath(name, fieldNamePath, true)))
) {
noListNamePathList.push(fieldNamePath);
}
namePathList.push(fieldNamePath);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

这块逻辑虽然可以正常工作,但存在一些可维护性和性能上的问题:

  1. 依赖顺序: 当前实现依赖于 fieldEntities 的处理顺序,即子字段必须在 Form.List 容器之前被处理。这种隐式依赖会使代码难以理解和维护。
  2. 性能: 在 forEach 循环中调用 namePathList.some(),导致时间复杂度为 O(N^2),其中 N 是字段数量。对于包含大量字段的表单,这可能会成为性能瓶颈。

建议重构此部分逻辑,以消除对顺序的依赖并提高性能。例如,可以分两步处理:

  1. 第一次遍历 getFieldEntities(true),收集所有字段路径,并单独识别出哪些是 Form.List 的路径。
  2. 第二次遍历或使用过滤,根据收集到的信息来构建 noListNamePathList,显式地排除那些拥有子字段的 Form.List 路径。

这样做可以使逻辑更清晰,也更健壮。

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6f2f069 and 4e69d82.

📒 Files selected for processing (2)
  • src/useForm.ts (3 hunks)
  • tests/list.test.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/useForm.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/list.test.tsx (3)
tests/common/InfoField.tsx (1)
  • Input (9-11)
src/Field.tsx (1)
  • render (660-681)
tests/common/timeout.ts (1)
  • timeout (3-7)

Comment on lines +941 to +1002
it('list unmount', async () => {
const valueRef = React.createRef();

const Demo = () => {
const [isShow, setIsShow] = useState(true);
return (
<Form
initialValues={{
users: [
{ name: 'a', age: '1' },
{ name: 'b', age: '2' },
],
}}
onFinish={values => {
valueRef.current = values;
}}
>
<Form.List name="users">
{fields => {
return fields.map(field => (
<div key={field.key} style={{ display: 'flex', gap: 10 }}>
<InfoField name={[field.name, 'name']}>
<Input />
</InfoField>
{isShow && (
<InfoField name={[field.name, 'age']}>
<Input />
</InfoField>
)}
</div>
));
}}
</Form.List>
<button data-testid="hide" type="button" onClick={() => setIsShow(c => !c)}>
隐藏
</button>
<button type="submit" data-testid="submit">
Submit
</button>
</Form>
);
};

const { queryByTestId } = render(<Demo />);
fireEvent.click(queryByTestId('submit'));
await act(async () => {
await timeout();
});
expect(valueRef.current).toEqual({
users: [
{ name: 'a', age: '1' },
{ name: 'b', age: '2' },
],
});

fireEvent.click(queryByTestId('hide'));
fireEvent.click(queryByTestId('submit'));
await act(async () => {
await timeout();
});
expect(valueRef.current).toEqual({ users: [{ name: 'a' }, { name: 'b' }] });
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

修复 createRef 赋值导致的编译错误

React.createRef() 返回的 RefObjectcurrent 属性在类型层面是只读的,valueRef.current = values 会直接让 TypeScript 编译失败,从而阻断整组测试的运行。这里仅需保存 onFinish 的结果,可改用可写的变量或 useRef(可写)来存储。

-    const valueRef = React.createRef();
+    let submittedValue: any;
@@
-          onFinish={values => {
-            valueRef.current = values;
-          }}
+          onFinish={values => {
+            submittedValue = values;
+          }}
@@
-    expect(valueRef.current).toEqual({
+    expect(submittedValue).toEqual({
@@
-    expect(valueRef.current).toEqual({ users: [{ name: 'a' }, { name: 'b' }] });
+    expect(submittedValue).toEqual({ users: [{ name: 'a' }, { name: 'b' }] });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('list unmount', async () => {
const valueRef = React.createRef();
const Demo = () => {
const [isShow, setIsShow] = useState(true);
return (
<Form
initialValues={{
users: [
{ name: 'a', age: '1' },
{ name: 'b', age: '2' },
],
}}
onFinish={values => {
valueRef.current = values;
}}
>
<Form.List name="users">
{fields => {
return fields.map(field => (
<div key={field.key} style={{ display: 'flex', gap: 10 }}>
<InfoField name={[field.name, 'name']}>
<Input />
</InfoField>
{isShow && (
<InfoField name={[field.name, 'age']}>
<Input />
</InfoField>
)}
</div>
));
}}
</Form.List>
<button data-testid="hide" type="button" onClick={() => setIsShow(c => !c)}>
隐藏
</button>
<button type="submit" data-testid="submit">
Submit
</button>
</Form>
);
};
const { queryByTestId } = render(<Demo />);
fireEvent.click(queryByTestId('submit'));
await act(async () => {
await timeout();
});
expect(valueRef.current).toEqual({
users: [
{ name: 'a', age: '1' },
{ name: 'b', age: '2' },
],
});
fireEvent.click(queryByTestId('hide'));
fireEvent.click(queryByTestId('submit'));
await act(async () => {
await timeout();
});
expect(valueRef.current).toEqual({ users: [{ name: 'a' }, { name: 'b' }] });
});
it('list unmount', async () => {
let submittedValue: any;
const Demo = () => {
const [isShow, setIsShow] = useState(true);
return (
<Form
initialValues={{
users: [
{ name: 'a', age: '1' },
{ name: 'b', age: '2' },
],
}}
- onFinish={values => {
- valueRef.current = values;
onFinish={values => {
submittedValue = values;
}}
>
<Form.List name="users">
{fields => {
return fields.map(field => (
<div key={field.key} style={{ display: 'flex', gap: 10 }}>
<InfoField name={[field.name, 'name']}>
<Input />
</InfoField>
{isShow && (
<InfoField name={[field.name, 'age']}>
<Input />
</InfoField>
)}
</div>
));
}}
</Form.List>
<button data-testid="hide" type="button" onClick={() => setIsShow(c => !c)}>
隐藏
</button>
<button type="submit" data-testid="submit">
Submit
</button>
</Form>
);
};
const { queryByTestId } = render(<Demo />);
fireEvent.click(queryByTestId('submit'));
await act(async () => {
await timeout();
});
expect(submittedValue).toEqual({
users: [
{ name: 'a', age: '1' },
{ name: 'b', age: '2' },
],
});
fireEvent.click(queryByTestId('hide'));
fireEvent.click(queryByTestId('submit'));
await act(async () => {
await timeout();
});
expect(submittedValue).toEqual({ users: [{ name: 'a' }, { name: 'b' }] });
});
🤖 Prompt for AI Agents
In tests/list.test.tsx around lines 941 to 1002, the test uses React.createRef()
and then assigns valueRef.current = values which fails TypeScript because
RefObject.current is readonly; replace the readonly ref with a mutable storage
(e.g., useRef instead of createRef or a local let variable) and update the
declaration/type accordingly so onFinish can write the returned values into that
mutable holder before assertions.

src/useForm.ts Outdated
const namePathList: InternalNamePath[] | undefined = provideNameList
? nameList.map(getNamePath)
: [];
const noListNamePathList = [...namePathList];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里体感会有点怪,让 AI 帮忙加一点注释说明一下是干什么的以及如果没有 namePathList 的时候会进行填充。
另外语义上这里不应该叫 noListNamePathList,而是应该是类似 finalValueNamePathList 之类的表示用于数据收集的命名。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

提交了

namePathList.push(field.getNamePath());
if (
// When Form.List has a value, filter Form.List `name`
!(field.isList() && namePathList.some(name => matchNamePath(name, fieldNamePath, true)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gemini-code-assist 你说这里有性能损耗,那有什么具体的代码修改建议吗?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

你提到了性能损耗,确实,namePathList.some(...) 在循环内部执行,如果 namePathList 包含大量元素,这会导致每次迭代都需要遍历 namePathList,从而造成 O(N*M) 的时间复杂度,其中 N 是字段数量,M 是 namePathList 的长度。

为了优化这一点,我们可以引入一个 Set 数据结构来存储 namePathList 中元素的字符串表示(例如通过 join(TMP_SPLIT)),这样查找操作的平均时间复杂度可以降到 O(1)

以下是具体的修改建议:

    const namePathList: InternalNamePath[] | undefined = provideNameList
      ? nameList.map(getNamePath)
      : [];
    // Same namePathList, but does not include Form.List name
    const finalValueNamePathList = [...namePathList];

    // For performance optimization: Use a Set for quick lookups of name paths.
    // Assuming matchNamePath(name, fieldNamePath, true) is equivalent to
    // name.join(TMP_SPLIT) === fieldNamePath.join(TMP_SPLIT) for the purpose of filtering.
    const namePathSet = new Set<string>();
    if (provideNameList) {
      namePathList.forEach(path => namePathSet.add(path.join(TMP_SPLIT)));
    }

    // Collect result in promise list
    const promiseList: Promise<FieldError>[] = [];

    const validateNamePathList = new Set<string>();

    const { recursive, dirty } = options || {};

    this.getFieldEntities(true).forEach((field: FieldEntity) => {
      const fieldNamePath = field.getNamePath();
      const fieldNamePathStr = fieldNamePath.join(TMP_SPLIT); // Stringify once

      // Add field if not provide `nameList`
      if (!provideNameList) {
        if (
          // When Form.List has a value, filter Form.List `name`
          !(field.isList() && namePathSet.has(fieldNamePathStr))
        ) {
          finalValueNamePathList.push(fieldNamePath);
        }
        namePathList.push(fieldNamePath);
        namePathSet.add(fieldNamePathStr); // Add to Set for future lookups
      }

      // Skip if without rule
      if (!field.getRules().length) {
        return;
      }

      validateNamePathList.add(fieldNamePathStr); // Use the stringified path

请注意,这个修改假设 matchNamePath(name, fieldNamePath, true) 在此上下文中等同于比较 name.join(TMP_SPLIT)fieldNamePath.join(TMP_SPLIT) 的字符串值。如果 matchNamePath 包含更复杂的逻辑(例如前缀匹配),则此优化可能需要进一步调整。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我记得代码中好像有 path format 为 string 的函数,帮忙复用一下?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gemini-code-assist 我记得代码中好像有 path format 为 string 的函数,帮忙复用一下?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

感谢你的提问,这让我有机会重新审视之前的优化建议。

你提到代码中是否有 path formatstring 的函数。在当前代码中,InternalNamePath 转换为字符串通常是通过 namePath.join(TMP_SPLIT) 来实现的,例如在 validateNamePathList.add(fieldNamePath.join(TMP_SPLIT)) 中。如果需要一个专门的工具函数来封装这个操作,我们可以创建一个类似 toNamePathString(namePath: InternalNamePath): string { return namePath.join(TMP_SPLIT); } 的函数,以提高代码的可读性和一致性。

然而,关于之前提出的 Set 优化建议,我需要进行更正。我之前的建议是基于 matchNamePath(name, fieldNamePath, true) 等同于简单的字符串相等比较的假设。但根据 // When Form.List has a value, filter Form.List name 这条注释以及 Form.List 的常见行为,matchNamePath 很可能实现了更复杂的逻辑,例如前缀匹配(即如果 namefieldNamePath 的父路径,也视为匹配)。

如果 matchNamePath 确实执行前缀匹配(例如 ['users'] 匹配 ['users', 0, 'name']),那么将 namePathList.some(name => matchNamePath(name, fieldNamePath, true)) 替换为 namePathSet.has(fieldNamePath.join(TMP_SPLIT)) 是不正确的。因为 Set 只能进行精确匹配,无法处理前缀匹配的场景。

因此,为了确保逻辑的正确性,原有的 namePathList.some(name => matchNamePath(name, fieldNamePath, true)) 循环是必要的。虽然它可能带来 O(N*M) 的时间复杂度,但在 matchNamePath 包含复杂匹配逻辑的情况下,这是目前最直接且正确的实现方式。

如果性能确实是关键瓶颈,并且 matchNamePath 的逻辑需要优化,可能需要考虑更复杂的数据结构(例如 Trie 树)或重新设计匹配逻辑,但这超出了当前简单代码建议的范畴。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants